feat(files_sharing): allow not deleting expiring shares #48225

skjnldsv · 2024-09-19T17:38:22Z

Fix #17996

Feature

Shares that expires are just deleted. Which leads to confusion.

We don't even know if the share existed or not....
With the new file request, we might want to extend the drop period time...
We need more transparency

Solution

Allow admins to disable share deletion on expiration (opt-in)
Adds a new view to list all expired shares

Status

Add admin setting
Add api controller
Add new view
Add integration tests

apps/files_sharing/lib/Controller/ExpiredShareAPIController.php

lib/private/Share20/Manager.php

apps/files_sharing/lib/Controller/DeletedShareAPIController.php

apps/files_sharing/lib/Controller/ExpiredShareAPIController.php

apps/files_sharing/lib/Controller/ShareAPIController.php

apps/files_sharing/lib/Controller/ShareApiControllerFactory.php

apps/files_sharing/lib/Controller/DeletedShareAPIController.php

apps/files_sharing/lib/Controller/ExpiredShareAPIController.php

apps/files_sharing/lib/Controller/ShareApiControllerFactory.php

apps/settings/lib/Settings/Admin/Sharing.php

 	 */
 	private function getHumanBooleanConfig(string $app, string $key, bool $default = false): bool {
-		return $this->config->getAppValue($app, $key, $default ? 'yes' : 'no') === 'yes';
+		return $this->config->getAppValue($app, $key, $default ? 'yes' : 'no') !== 'no';


skjnldsv · 2024-10-09T17:15:25Z

All green and ready 🚀

come-nc

Reviewed PHP changes only

apps/files_sharing/lib/Controller/ShareApiControllerFactory.php

apps/files_sharing/appinfo/routes.php

apps/files_sharing/lib/Controller/DeletedShareAPIController.php

apps/files_sharing/lib/Controller/ExpiredShareAPIController.php

apps/files_sharing/lib/Controller/ShareAPIController.php

apps/files_sharing/lib/Controller/ExpiredShareAPIController.php

apps/files_sharing/lib/ExpireSharesJob.php

lib/private/Share20/Manager.php

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

skjnldsv · 2024-10-16T06:32:18Z

Addressed @come-nc

come-nc · 2024-10-17T08:29:53Z

apps/files_sharing/lib/Controller/ShareApiControllerFactory.php

+/**
+ * @psalm-import-type Files_SharingShare from ResponseDefinitions
+ */
+abstract class ShareApiControllerFactory extends OCSController {


Suggested change

abstract class ShareApiControllerFactory extends OCSController {

abstract class ShareApiControllerBase extends OCSController {

I’m still unhappy with the name, this is not a factory.
See https://en.wikipedia.org/wiki/Factory_method_pattern
A Factory has static methods to create instances. This here is simply a base to be used as parent by other controllers.

come-nc · 2024-10-17T08:32:16Z

apps/files_sharing/lib/Controller/DeletedShareAPIController.php

+		protected ShareManager $shareManager,
+		protected ?string $userId,
+		protected IUserManager $userManager,
+		protected IGroupManager $groupManager,
+		protected IRootFolder $rootFolder,
+		protected IAppManager $appManager,
+		protected ContainerInterface $serverContainer,
+		protected UserStatusManager $userStatusManager,
+		protected IPreview $previewManager,
+		protected IDateTimeZone $dateTimeZone,
+		protected IURLGenerator $urlGenerator,
+		protected IL10N $l,
+		protected LoggerInterface $logger,


Suggested change

protected ShareManager $shareManager,

protected ?string $userId,

protected IUserManager $userManager,

protected IGroupManager $groupManager,

protected IRootFolder $rootFolder,

protected IAppManager $appManager,

protected ContainerInterface $serverContainer,

protected UserStatusManager $userStatusManager,

protected IPreview $previewManager,

protected IDateTimeZone $dateTimeZone,

protected IURLGenerator $urlGenerator,

protected IL10N $l,

protected LoggerInterface $logger,

ShareManager $shareManager,

?string $userId,

IUserManager $userManager,

IGroupManager $groupManager,

IRootFolder $rootFolder,

IAppManager $appManager,

ContainerInterface $serverContainer,

UserStatusManager $userStatusManager,

IPreview $previewManager,

IDateTimeZone $dateTimeZone,

IURLGenerator $urlGenerator,

IL10N $l,

LoggerInterface $logger,

come-nc · 2024-10-17T08:33:14Z

apps/files_sharing/lib/Controller/ExpiredShareAPIController.php

+	public function __construct(
+		IRequest $request,
+		ShareManager $shareManager,
+		?string $userId,
+		IUserManager $userManager,
+		IGroupManager $groupManager,
+		IRootFolder $rootFolder,
+		IAppManager $appManager,
+		ContainerInterface $serverContainer,
+		UserStatusManager $userStatusManager,
+		IPreview $previewManager,
+		IDateTimeZone $dateTimeZone,
+		IURLGenerator $urlGenerator,
+		IL10N $l,
+		LoggerInterface $logger,
+	) {
+		parent::__construct(
+			$request,
+			$shareManager,
+			$userId,
+			$userManager,
+			$groupManager,
+			$rootFolder,
+			$appManager,
+			$serverContainer,
+			$userStatusManager,
+			$previewManager,
+			$dateTimeZone,
+			$urlGenerator,
+			$l,
+			$logger,
+		);
+	}


Suggested change

public function __construct(

IRequest $request,

ShareManager $shareManager,

?string $userId,

IUserManager $userManager,

IGroupManager $groupManager,

IRootFolder $rootFolder,

IAppManager $appManager,

ContainerInterface $serverContainer,

UserStatusManager $userStatusManager,

IPreview $previewManager,

IDateTimeZone $dateTimeZone,

IURLGenerator $urlGenerator,

IL10N $l,

LoggerInterface $logger,

) {

parent::__construct(

$request,

$shareManager,

$userId,

$userManager,

$groupManager,

$rootFolder,

$appManager,

$serverContainer,

$userStatusManager,

$previewManager,

$dateTimeZone,

$urlGenerator,

$l,

$logger,

);

}

This one is really empty or did I miss something?

come-nc · 2024-10-17T08:37:07Z

apps/files_sharing/lib/Controller/ShareAPIController.php

+		protected IManager $shareManager,
+		protected IGroupManager $groupManager,
+		protected IUserManager $userManager,
+		protected IRootFolder $rootFolder,
+		protected IURLGenerator $urlGenerator,
+		protected IL10N $l,
+		protected IConfig $config,
+		protected IAppManager $appManager,
+		protected ContainerInterface $serverContainer,
+		protected IUserStatusManager $userStatusManager,
+		protected IPreview $previewManager,
+		protected IDateTimeZone $dateTimeZone,
+		protected LoggerInterface $logger,
+		protected IProviderFactory $factory,
+		protected IMailer $mailer,
+		string $userId,


Suggested change

protected IManager $shareManager,

protected IGroupManager $groupManager,

protected IUserManager $userManager,

protected IRootFolder $rootFolder,

protected IURLGenerator $urlGenerator,

protected IL10N $l,

protected IConfig $config,

protected IAppManager $appManager,

protected ContainerInterface $serverContainer,

protected IUserStatusManager $userStatusManager,

protected IPreview $previewManager,

protected IDateTimeZone $dateTimeZone,

protected LoggerInterface $logger,

protected IProviderFactory $factory,

protected IMailer $mailer,

string $userId,

IManager $shareManager,

IGroupManager $groupManager,

IUserManager $userManager,

IRootFolder $rootFolder,

IURLGenerator $urlGenerator,

IL10N $l,

protected IConfig $config,

IAppManager $appManager,

ContainerInterface $serverContainer,

IUserStatusManager $userStatusManager,

IPreview $previewManager,

IDateTimeZone $dateTimeZone,

LoggerInterface $logger,

protected IProviderFactory $factory,

protected IMailer $mailer,

?string $userId,

Ideally I would also reorder to keep promoted properties together, I would say at the end (so, start with parameters from parent constructor in the same order as parent, add you own after)

come-nc · 2024-10-17T08:39:06Z

apps/files_sharing/lib/Controller/ShareApiControllerFactory.php

+		protected LoggerInterface $logger,
+	) {
+		parent::__construct(Application::APP_ID, $request);
+		$this->currentUser = $userId;


Suggested change

$this->currentUser = $userId;

Is there any added value to duplicate $this->userId into $this->currentUser?
If this is to have a clearer name, then remove the protected in front of $userId parameter to not turn it into a property, and keep $this->currentUser.

It's here because it was used by the controllers before this pr.
I want to keep changes to the existing parts to a minimal.

This was changed on master, so you should update your PR accordingly.

Thanks, I need to take that PR back 🙈

apps/files_sharing/lib/ExpireSharesJob.php

joshtrichards · 2024-11-07T15:16:42Z

Other issues this may address (did not link since I wasn't certain, but do so if appropriate please 😄 ):

Related too:

nfebe · 2024-11-08T17:07:44Z

apps/files_sharing/lib/Controller/ExpiredShareAPIController.php

+		IRequest $request,
+		ShareManager $shareManager,
+		?string $userId,
+		IUserManager $userManager,
+		IGroupManager $groupManager,
+		IRootFolder $rootFolder,
+		IAppManager $appManager,
+		ContainerInterface $serverContainer,
+		UserStatusManager $userStatusManager,
+		IPreview $previewManager,
+		IDateTimeZone $dateTimeZone,
+		IURLGenerator $urlGenerator,
+		IL10N $l,
+		LoggerInterface $logger,


Where are these properties and their access modifiers defined?

nfebe · 2024-11-08T17:25:14Z

apps/files_sharing/lib/Controller/ExpiredShareAPIController.php

+		$groupShares = $this->shareManager->getExpiredShares($this->userId, IShare::TYPE_GROUP, null, -1, 0);
+		$roomShares = $this->shareManager->getExpiredShares($this->userId, IShare::TYPE_ROOM, null, -1, 0);
+		$deckShares = $this->shareManager->getExpiredShares($this->userId, IShare::TYPE_DECK, null, -1, 0);
+		$sciencemeshShares = $this->shareManager->getExpiredShares($this->userId, IShare::TYPE_SCIENCEMESH, null, -1, 0);
+		$linkShares = $this->shareManager->getExpiredShares($this->userId, IShare::TYPE_LINK, null, -1, 0);
+		$userShares = $this->shareManager->getExpiredShares($this->userId, IShare::TYPE_USER, null, -1, 0);
+		$emailsShares = $this->shareManager->getExpiredShares($this->userId, IShare::TYPE_EMAIL, null, -1, 0);
+		$circlesShares = $this->shareManager->getExpiredShares($this->userId, IShare::TYPE_CIRCLE, null, -1, 0);
+		$remoteShares = $this->shareManager->getExpiredShares($this->userId, IShare::TYPE_REMOTE, null, -1, 0);


No way to get all? shareType -1, could be default parameter in share Manager that returns all?

nfebe · 2024-11-08T17:25:22Z

apps/files_sharing/lib/Controller/ExpiredShareAPIController.php

+		$groupShares = $this->shareManager->getExpiredShares($this->userId, IShare::TYPE_GROUP, null, -1, 0);
+		$roomShares = $this->shareManager->getExpiredShares($this->userId, IShare::TYPE_ROOM, null, -1, 0);
+		$deckShares = $this->shareManager->getExpiredShares($this->userId, IShare::TYPE_DECK, null, -1, 0);
+		$sciencemeshShares = $this->shareManager->getExpiredShares($this->userId, IShare::TYPE_SCIENCEMESH, null, -1, 0);
+		$linkShares = $this->shareManager->getExpiredShares($this->userId, IShare::TYPE_LINK, null, -1, 0);
+		$userShares = $this->shareManager->getExpiredShares($this->userId, IShare::TYPE_USER, null, -1, 0);
+		$emailsShares = $this->shareManager->getExpiredShares($this->userId, IShare::TYPE_EMAIL, null, -1, 0);
+		$circlesShares = $this->shareManager->getExpiredShares($this->userId, IShare::TYPE_CIRCLE, null, -1, 0);
+		$remoteShares = $this->shareManager->getExpiredShares($this->userId, IShare::TYPE_REMOTE, null, -1, 0);


No way to get all? shareType -1, could be default parameter in share Manager that returns all?

nfebe · 2024-11-08T17:31:04Z

apps/files_sharing/lib/ExpireSharesJob.php

@@ -42,13 +41,16 @@ public function __construct(ITimeFactory $time, IManager $shareManager, IDBConne
 	 * @param array $argument unused argument
 	 */
 	public function run($argument) {
-		//Current time
+		if ($this->config->getValueString('core', 'shareapi_delete_on_expire', 'yes') === 'no') {


I believe this would be a thing for config.php... We should

Update config.sample.php that is an in code source documentation

Document this somewhere the administration manual

nfebe · 2024-11-08T17:33:23Z

apps/files_sharing/lib/Controller/DeletedShareAPIController.php

-	private $serverContainer;
-
-	public function __construct(string $appName,
+class DeletedShareAPIController extends ShareApiControllerFactory {


Nitpick : Is this BaseShareApiController or Factory? Factories are to help easy object creation, testing, decoupling not exactly DRY-ing?

skjnldsv added enhancement 2. developing Work in progress feature: sharing labels Sep 19, 2024

skjnldsv added this to the Nextcloud 31 milestone Sep 19, 2024

skjnldsv self-assigned this Sep 19, 2024

github-advanced-security bot found potential problems Sep 19, 2024

View reviewed changes

skjnldsv requested a review from provokateurin as a code owner September 20, 2024 10:07

github-advanced-security bot found potential problems Sep 20, 2024

View reviewed changes

skjnldsv force-pushed the feat/disable-share-deletion branch from 85a6fa2 to 1cbec70 Compare September 20, 2024 10:24

github-advanced-security bot found potential problems Sep 20, 2024

View reviewed changes

skjnldsv force-pushed the feat/disable-share-deletion branch 3 times, most recently from 099984d to 98e0fe3 Compare September 20, 2024 10:52

github-advanced-security bot found potential problems Sep 20, 2024

View reviewed changes

skjnldsv force-pushed the feat/disable-share-deletion branch from 98e0fe3 to 3f32873 Compare September 20, 2024 11:27

skjnldsv force-pushed the feat/disable-share-deletion branch 8 times, most recently from d4aab4b to fda83da Compare October 9, 2024 16:19

skjnldsv requested review from susnux, nfebe, a team and Pytal and removed request for a team October 9, 2024 17:15

skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 9, 2024

skjnldsv force-pushed the feat/disable-share-deletion branch from fda83da to b436212 Compare October 10, 2024 06:36

come-nc requested changes Oct 15, 2024

View reviewed changes

skjnldsv force-pushed the feat/disable-share-deletion branch from 8274d2a to 2eaedc6 Compare October 15, 2024 13:44

skjnldsv added 2 commits October 15, 2024 16:40

feat(files_sharing): allow not deleting expiring shares

06069e5

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

chore(files_sharing): refactor ShareAPIController shared methods

fc3ae5c

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

skjnldsv force-pushed the feat/disable-share-deletion branch from 2eaedc6 to fc3ae5c Compare October 15, 2024 15:07

skjnldsv requested a review from come-nc October 16, 2024 06:32

come-nc reviewed Oct 17, 2024

View reviewed changes

skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 29, 2024

joshtrichards mentioned this pull request Nov 7, 2024

Handle expired links better to avoid false positives nextcloud/bruteforcesettings#562

Open

nfebe reviewed Nov 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(files_sharing): allow not deleting expiring shares #48225

feat(files_sharing): allow not deleting expiring shares #48225

skjnldsv commented Sep 19, 2024 •

edited

Loading

skjnldsv commented Oct 9, 2024

come-nc left a comment

skjnldsv commented Oct 16, 2024

come-nc Oct 17, 2024

come-nc Oct 17, 2024

come-nc Oct 17, 2024

come-nc Oct 17, 2024

come-nc Oct 17, 2024

skjnldsv Oct 18, 2024

provokateurin Nov 8, 2024

skjnldsv Nov 8, 2024

joshtrichards commented Nov 7, 2024

nfebe Nov 8, 2024

nfebe Nov 8, 2024

nfebe Nov 8, 2024

nfebe Nov 8, 2024

nfebe Nov 8, 2024

	abstract class ShareApiControllerFactory extends OCSController {
	abstract class ShareApiControllerBase extends OCSController {

feat(files_sharing): allow not deleting expiring shares #48225

Are you sure you want to change the base?

feat(files_sharing): allow not deleting expiring shares #48225

Conversation

skjnldsv commented Sep 19, 2024 • edited Loading

Feature

Solution

Status

skjnldsv commented Oct 9, 2024

come-nc left a comment

Choose a reason for hiding this comment

skjnldsv commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshtrichards commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skjnldsv commented Sep 19, 2024 •

edited

Loading